-
-
Notifications
You must be signed in to change notification settings - Fork 32.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[material-ui][Badge] Deprecated components and componentsProps props for v6 #41399
Conversation
…j/badge-v6-deprecations
Netlify deploy previewBundle size reportBundle size will be reported once CircleCI build #674699 finishes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delay in the review.
Let me know if you need help with any of these 😊
@@ -283,31 +283,8 @@ describe('<Badge />', () => { | |||
}); | |||
}); | |||
|
|||
describe('prop: components / slots', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to delete this, we will delete it when the prop is removed.
@@ -331,23 +308,7 @@ describe('<Badge />', () => { | |||
}); | |||
}); | |||
|
|||
describe('prop: componentsProps / slotProps', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to delete this, we will delete it when the prop is removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should also add the theme tests, like this one: https://github.com/mui/material-ui/blob/master/packages/mui-codemod/src/deprecations/slider-props/slider-props.test.js#L29
packages/mui-codemod/src/deprecations/badge-props/test-cases/excepted.js
Outdated
Show resolved
Hide resolved
|
||
slotProps: { | ||
root: slotsRootProps, | ||
badge: slotsbadgeProps, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to update this one as well. Same as my previous comment (#41399 (comment)).
@DiegoAndai made changes as told by you. please review and give feedback |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @skmanoj322! Thanks for working on this. Here are some review comments:
packages/mui-codemod/src/deprecations/badge-props/test-cases/actual.js
Outdated
Show resolved
Hide resolved
packages/mui-codemod/src/deprecations/badge-props/test-cases/actual.js
Outdated
Show resolved
Hide resolved
packages/mui-codemod/src/deprecations/badge-props/test-cases/expected.js
Outdated
Show resolved
Hide resolved
packages/mui-codemod/src/deprecations/badge-props/test-cases/expected.js
Outdated
Show resolved
Hide resolved
packages/mui-codemod/src/deprecations/badge-props/test-cases/expected.js
Outdated
Show resolved
Hide resolved
Hey @DiegoAndai Thank you for bearing with me . I have refered the slider and alert component and implemented the same here. just a short question is there any particular logic to write these test (actual.js) and , |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @skmanoj322!
To fix the failing test cases:
- Running
pnpm prettier
should modify the files with errors, and then commit and push the changes - I left code suggestions to fix the codemod test failures
just a short question is there any particular logic to write these test (actual.js)
The idea is to cover all ways users might be using the deprecated props. Then you should be able to run
node packages/mui-codemod/codemod deprecations/badge-props packages/mui-codemod/src/deprecations/badge-props/test-cases/actual.js
And that would modify actual.js
, then you copy the modified code to expected.js
and undo the modification of actual.js
pnpm tc i tried to implement the command from the root terminal it gave me error
Did you run this?:
pnpm tc badge-props
What error did it throw?
it('should be idempotent', () => { | ||
const actual = transform({ source: read('./test-cases/actual.js') }, { jscodeshift }, {}); | ||
|
||
const excepted = read('./test-cases/excepted.js'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const excepted = read('./test-cases/excepted.js'); | |
const excepted = read('./test-cases/expected.js'); |
it('transforms props as needed', () => { | ||
const actual = transform({ source: read('./test-cases/actual.js') }, { jscodeshift }, {}); | ||
|
||
const expected = read('./test-cases/excepted.js'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const expected = read('./test-cases/excepted.js'); | |
const expected = read('./test-cases/expected.js'); |
describe('deprecations', () => { | ||
describe('badge-props', () => { | ||
it('transforms props as needed', () => { | ||
const actual = transform({ source: read('./test-cases/actual.js') }, { jscodeshift }, {}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const actual = transform({ source: read('./test-cases/actual.js') }, { jscodeshift }, {}); | |
const actual = transform({ source: read('./test-cases/actual.js') }, { jscodeshift }, { printOptions: { trailingComma: true } }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these is missing in slider any specific reason why?
hey @DiegoAndai tried to run these test
and for
btw really appricate your mentoring thanks for the guidence 🎉 👍 |
Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>
Co-authored-by: José Rodolfo Freitas <joserodolfo.freitas@gmail.com> Co-authored-by: Alexandre Fauquette <45398769+alexfauquette@users.noreply.github.com> Co-authored-by: Flavien DELANGLE <flaviendelangle@gmail.com> Co-authored-by: Danilo Leal <67129314+danilo-leal@users.noreply.github.com> Co-authored-by: Bilal Shafi <bilalshafidev@gmail.com>
merging master
@DiegoAndai sorry i think there is a lot to merge conflit i have to resolve that . again sorry for trouble again 😞 . i thing there is lot of code changes that have happened . shall i just create new PR? Or
|
Closing in favor of #41655 |
@skmanoj322 sorry I was late in replying to this PR. The issue that caused so many files to change is that we recently switched to the
Instad of master, we should've merged |
@DiegoAndai part of #41279